fix: decode invalid UTF-8 at the JVM to native FFI import boundary#4945
Draft
andygrove wants to merge 6 commits into
Draft
fix: decode invalid UTF-8 at the JVM to native FFI import boundary#4945andygrove wants to merge 6 commits into
andygrove wants to merge 6 commits into
Conversation
…d-type test coverage
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #4764 (EPIC: consistent handling of invalid UTF-8 in native StringType). This PR implements Gap B only, decoding invalid UTF-8 at the JVM to native Arrow FFI import boundary. It does not close the EPIC, which still tracks Gap A (the native scan rejecting invalid UTF-8).
Rationale for this change
Spark's
StringType(UTF8String) can hold arbitrary bytes, including sequences that are not valid UTF-8. When a JVM side source hands string columns to native code over the Arrow C Data Interface, arrow-rs imports them withfrom_ffi/from_ffi_and_data_type, which build the array viaArrayData::new_uncheckedand do not validate UTF-8. The imported ArrowUtf8/LargeUtf8array then lies about its validity, and any downstream native string kernel that reads&strthrough arrow-rs's uncheckedStringArray::value()(from_utf8_unchecked) exercises undefined behaviour: iterating chars, slicing on char boundaries, and similar operations can misbehave, panic, or be miscompiled. This is a latent, default configuration soundness hazard.The string producing sites already decode invalid bytes the way Spark renders them:
CAST(binary AS string)(#4763) and native shuffleget_string(#4521) both usedecode_utf8_spark_lossy. This PR applies the same policy to the string ingress side, so the whole native pipeline agrees on a single invariant: native string data is always valid UTF-8.What changes are included in this PR?
decode_string_arrayswalker indatafusion-comet-common, beside the existingdecode_utf8_spark_lossy. It ensures everyUtf8/LargeUtf8array reachable from an imported column holds valid UTF-8, decoding invalid bytes to Spark's rendered form. It is zero copy for the valid common case (a singlefrom_utf8validation pass plus an O(number of strings) boundary check, returning the sameArc), and rebuilds element by element only when bytes are genuinely invalid. It recurses throughDictionary,Struct,List,LargeList,FixedSizeList, andMap.ScanExec::pull_next, which handles all native query input (the native_comet JVM reader, Spark columnar handoff, shuffle reads, mapInArrow). Decoding runs before dictionary unpack so compact dictionary values are validated rather than the expanded ones.columnarToRowconversion."é"(bytesC3 A9) split across two offsets would otherwise hand each element an invalid slice, andvalue()decodes those unchecked.No configuration flag gates this. It fixes undefined behaviour in the default configuration and matches the always on behaviour of the sibling cast and shuffle fixes.
The only observable divergence from Spark is the previously documented one: decoding rather than preserving raw bytes differs only under byte level round trips (for example
CAST(CAST(X'FF' AS STRING) AS BINARY)), already noted in the compatibility guide.How are these changes tested?
Dictionary/Struct/List/FixedSizeList/Map(decode and zero copy paths); null preservation; sliced arrays with a non zero starting offset; and trailing empty strings.ScanExecimport site proving an invalid UTF-8 column is decoded through the production per column path.